Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add support for detecting AWS Advanced JDBC Wrapper to DatabaseDriver #43812

Closed
wants to merge 1 commit into from
Closed

Add support for detecting AWS Advanced JDBC Wrapper to DatabaseDriver #43812

wants to merge 1 commit into from
+29 −3

Conversation

vpavic
Copy link
Contributor

@vpavic vpavic commented Jan 14, 2025

This commit adds entry for AWS Advanced JDBC Wrapper to DatabaseDriver enum.

See gh-31995

@vpavic
Add support for AWS Advanced JDBC Wrapper
0b00827 
This commit adds entry for AWS Advanced JDBC Wrapper to `DatabaseDriver`
enum.

See spring-projectsgh-31995

Signed-off-by: Vedran Pavic <[email protected]>
@vpavic vpavic mentioned this pull request Jan 14, 2025
Closed
@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Jan 14, 2025
vpavic
vpavic commented Jan 14, 2025
View reviewed changes
spring-boot-project/spring-boot/src/main/java/org/springframework/boot/jdbc/DatabaseDriver.java

@Override
protected Collection<String> getUrlPrefixes() {
return List.of("aws-wrapper");
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've used List#of as it seems to be the simplest option, but note that implementations of #getUrlPrefixes in this enum are quite inconsistent - there are usages of Arrays#asList and Collections#singleton.

Perhaps this could be aligned in a separate issue to use List#of consistently (first-timers-only candidate?).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They seem consistent to me, using Collections#singleton when there's a single prefix and Arrays#asList when there a multiple prefixes. I'll polish to use Collections#singleton when merging.

@wilkinsona
Copy link
Member

Thanks, @vpavic. In isolation, I wonder how useful this will be. Looking at how DatabaseDriver is used, being able to detect AWS's wrapper may not be enough in some cases. Take Flyway's auto-configuration for example. As you may remember as I think you contributed it, it uses DatabaseDriver to figure out which vendor-specific migrations to use. In that situation, I think we'd need to detect the driver that's being wrapped rather than the wrapper. I think the same applies to the basic script-based initialization and the like as well.

@wilkinsona wilkinsona added the status: waiting-for-feedback We need additional information before we can continue label Jan 15, 2025
@vpavic
Copy link
Contributor Author

vpavic commented Jan 15, 2025

Thanks for the feedback.

Motivation here is to simplify usage of AWS Advanced JDBC Wrapper by not having to specify spring.datasource.driver-class-name property. Depending on the environment (and the exact database used), sometimes we need to use plain PostgreSQL JDBC driver and other times AWS wrapper - it would be very useful to do that switch only by changing a single configuration property.

I understand your concerns, and have refreshed my memory a bit on support for vendor-specific Flyway migrations (yes, I contributed that quite some while ago 🙂) and I believe your comment aims at this usage of DatabaseDriver:

spring-boot/spring-boot-project/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/flyway/FlywayAutoConfiguration.java

Lines 391 to 392 in 269c761

String url = JdbcUtils.extractDatabaseMetaData(this.dataSource, DatabaseMetaData::getURL);
return DatabaseDriver.fromJdbcUrl(url);

This indeed doesn't play nice with wrapper drivers but such use cases could be updated to rely on database product name instead. I can contribute that if you agree with that direction.

I can see that such approach is already used in PlatformPlaceholderDatabaseDriverResolver:

spring-boot/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/jdbc/init/PlatformPlaceholderDatabaseDriverResolver.java

Lines 139 to 141 in 269c761

String productName = JdbcUtils.commonDatabaseName(
JdbcUtils.extractDatabaseMetaData(dataSource, DatabaseMetaData::getDatabaseProductName));
return DatabaseDriver.fromProductName(productName);

In any case, I think these concerns are not new since Testcontainers JDBC driver also is of similar nature as it wraps the underlying JDBC driver and requires inserting its prefix into JDBC URL.

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Jan 15, 2025
@wilkinsona
Copy link
Member

wilkinsona commented Jan 15, 2025 •
edited
Loading

Thanks, @vpavic. After experimenting a bit with the Testcontainers JDBC driver, I learned that it isn't affected as it delegates the metadata request to the underlying driver. Having learned that, I tried the same with the AWS wrapper and it behaves the same way so the problem that I had anticipated does not occur.

To summarise:

  • Adding the AWS wrapper to DatabaseDriver will remove the need to specify software.amazon.jdbc.Driver as the driver class name
  • Code that uses DatabaseMetaData will continue to work as the wrapper Driver delegates the metadata request to the underlying driver

@wilkinsona wilkinsona changed the title Add support for AWS Advanced JDBC Wrapper Add support for detecting AWS Advanced JDBC Wrapper to DatabaseDriver Jan 15, 2025
@wilkinsona wilkinsona added type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged status: feedback-provided Feedback has been provided labels Jan 15, 2025
@wilkinsona wilkinsona self-assigned this Jan 15, 2025
@wilkinsona wilkinsona modified the milestones: 3.5.x, 3.5.0-M1 Jan 15, 2025
wilkinsona pushed a commit that referenced this pull request Jan 15, 2025
@vpavic @wilkinsona
Add support for AWS Advanced JDBC Wrapper
2bf413c 
This commit adds an entry for the AWS Advanced JDBC Wrapper to the
DatabaseDriver enum. This allows the driver class name to be
auto-detected from jdbc:aws-wrapper:… URLs.

See gh-43812

Signed-off-by: Vedran Pavic <[email protected]>
wilkinsona added a commit that referenced this pull request Jan 15, 2025
@wilkinsona
Polish "Add support for AWS Advanced JDBC Wrapper"
d90e622 
See gh-43812
@wilkinsona
Copy link
Member

Thanks very much, @vpavic.

@vpavic vpavic deleted the gh-31995 branch January 15, 2025 19:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wilkinsona wilkinsona wilkinsona left review comments

Assignees

@wilkinsona wilkinsona

Labels
type: enhancement A general enhancement
Projects
None yet
Milestone
3.5.0-M1
Development

Successfully merging this pull request may close these issues.
3 participants
@vpavic @wilkinsona @spring-projects-issues